Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Only use pseudo-handle for coverage dbghelp context #918

Merged
merged 2 commits into from
May 24, 2021

Conversation

ranweiler
Copy link
Member

@ranweiler ranweiler commented May 23, 2021

Closes #914.

Summary

When recording coverage, don't try to use live process handles as the context for the dbghelp symbol handler in the module feature cache. Instead, use the default pseudo-handle for the semantically static PDB search, and the target process handle for any queries about the target and its address space.

Details

The docs for SymInitializeW from dbghelp are a bit unclear on the exact constraints (and failure modes) around the HANDLE one uses to interact with the symbol handler in all dbghelp functions.

The docs for the HANDLE hProcess parameter say:

A handle that identifies the caller. This value should be unique and nonzero, but need not be a process handle. However, if you do use a process handle, be sure to use the correct handle. If the application is a debugger, use the process handle for the process being debugged. Do not use the handle returned by GetCurrentProcess when debugging another process, because calling functions like SymLoadModuleEx can have unexpected results.

Because the coverage recorder is debugging the target process, we follow this guidance. We (schedule!) initialization of the symbol handler on CREATE_PROCESS_DEBUG_EVENT, and try to use the process's handle as the symbol handler context when searching for the PDB.

However, this doesn't really seem to be necessary, adds some complications, and doesn't work when we do our PDB search:

  1. Our call to maybe_sym_initialize() avoids double-dbghelp init, but only forces our dbghelp wrapper to internally schedule initialization (pending observation of the process's initial breakpoint). But in coverage recording, we try to use the handle as a symbol handler context immediately, before actual symbol handler init, when searching for a PDB associated with the process's executable module. This then fails, and we cannot extract coverage features for the new process.
  2. Due to dynamic loading, we also can't assume that a given process execution is associated with a fixed (or even statically-knowable) set of modules. That means we can't just do all feature extraction statically (i.e. before observing CREATE_PROCESS_DEBUG_EVENT), which would guarantee we aren't using the "wrong" (pseudo-)handle.
  3. To only use the debuggee handle for PDB search, we'd have to delay initial feature extraction until then, too.

It appears we can sidestep this issue by exclusively using the custom, (non-GetCurrentProcess()) pseudo-handle in coverage::pdb when using dbghelp for PDB search. As required by the docs, this uniquely "identifies the caller", perhaps at the cost of redundant handler init.

In the future, we should revisit our dbghelp wrapper to have a more explicit notion of context that hides the inherited HANDLE, and presents a misuse-resistant API (captured in #917).

@ranweiler ranweiler merged commit 2c9a73d into microsoft:main May 24, 2021
@ranweiler ranweiler deleted the cov-sym-init branch May 24, 2021 15:37
@ghost ghost locked as resolved and limited conversation to collaborators Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breakpoint refactor broke dbghelp usage in coverage
2 participants